refactor: Build RBAC with recommended labels#821
Conversation
0fbde39 to
6866383
Compare
|
|
||
| /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount shared by all | ||
| /// Pods, its (namespaced) RoleBinding, and the operator-deployed ClusterRole it binds. | ||
| pub fn rbac_resource_names(&self) -> role_utils::ResourceNames { |
There was a problem hiding this comment.
When I read this, the following questions came to my mind: Why are there two resource-name functions? Why can't a single function provide the resource names for all resources? What makes the RBAC resources so special? In reality, though, the situation is that rbac_resource_names provides the names for the resources at the cluster level, whereas resource_names provides the names for the role-group resources. The fact that rbac_resource_names (currently) only provides the names for RBAC resources is more of a coincidence. In the future it could, for example, also provide the name for the discovery ConfigMap. I would either name the two functions cluster_resource_names and role_group_resource_names (or something along those lines), or somehow combine them both.
The OpenSearch operator does not have this problem because it has a separate context for the role resources (https://github.com/stackabletech/opensearch-operator/blob/26.7.0/rust/operator-binary/src/controller/build/role_builder.rs#L48) and the role-group resources (https://github.com/stackabletech/opensearch-operator/blob/26.7.0/rust/operator-binary/src/controller/build/role_group_builder.rs#L210) respectively. That could be another way to solve it.
(The OpenSearch CRD has only one role, so the cluster-level resources were accidentally moved into the RoleBuilder which is wrong and should be changed.)
There was a problem hiding this comment.
Renamed to cluster_resource_names and role_group_resource_names: 9ef8a4b
| // The group listener is a role-level object, so a constant `none` role-group is used as the | ||
| // role-group label value. | ||
| stackable_operator::constant!(PLACEHOLDER_LISTENER_ROLE_GROUP: RoleGroupName = "none"); |
There was a problem hiding this comment.
Is this placeholder there only for backwards-compatibility or is this the desired solution?
There was a problem hiding this comment.
Yes, this was inconsistent naming. Renamed here to what we have elsewhere: 856af92 (the constant value itself was already used as such in the pre-PR code)
| /// Both resources are shared by the whole cluster rather than tied to a role or role group, so | ||
| /// the recommended labels carry `none` for both values. | ||
| fn rbac_labels(cluster: &ValidatedCluster) -> Labels { | ||
| cluster.recommended_labels_for(&NONE_ROLE_NAME, &NONE_ROLE_GROUP_NAME) |
There was a problem hiding this comment.
There should be variants for recommended_labels_for specific to role and cluster resources.
There was a problem hiding this comment.
As discussed, we can keep it as it is for now, as all 12 operators should be consistent and the PRs a little easier to review. We can make this change (plus the one in your comment below) when we move the labels stuff into op-rs in a subsequent iteration. I've added it to the TODOs.
There was a problem hiding this comment.
If the labels are always the recommended ones, then we could just create the labels in rbac::build_service_account and rbac::build_role_binding. Once rbac_labels() is gone, there is no longer any reason for this file to exist, because it merely forwards function calls without making any noteworthy contribution.
| role.to_string() | ||
| .parse() | ||
| .expect("an AirflowRole serialises to a valid RoleName") |
There was a problem hiding this comment.
This assumption can easily be checked in a unit test by iterating over the variants of AirflowRole.
2 participants
Description
Part of: stackabletech/issues#869
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker